Conversation
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
sagikazarmark
left a comment
sagikazarmark
left a comment
There was a problem hiding this comment.
Thanks @nabokihms !
I had a few requests, but it's great to finally have graceful shutdown in Dex!
cmd/dex/serve.go
Outdated
|
|
||
| return <-errc | ||
| sig := make(chan os.Signal) | ||
| gr.Add(func() error { |
There was a problem hiding this comment.
The library has a builtin signal actor, maybe we could use it.
There was a problem hiding this comment.
Fixed. The only thing that I dislike about helper is that I can't log the received signal immediately.
cmd/dex/serve.go
Outdated
| if err := l.Close(); err != nil { | ||
| logger.Errorf("gracefully close (%s) listener: %v", name, err) | ||
| } |
There was a problem hiding this comment.
AFAIK the http server takes care of this, so you don't need to call close on the listener manually.
There was a problem hiding this comment.
My thought was about splitting listener closing error and server shutdown error. Maybe it is excessive.
There was a problem hiding this comment.
Listener closing now is a duty of a server.
cmd/dex/serve.go
Outdated
| if err := grpcListener.Close(); err != nil { | ||
| logger.Errorf("failed to gracefully close (grpc) listener: %v", err) | ||
| } |
| err := http.ListenAndServe(c.Telemetry.HTTP, telemetryServ) | ||
| errc <- fmt.Errorf("listening on %s failed: %v", c.Telemetry.HTTP, err) | ||
| }() | ||
| telemetrySrv := &http.Server{Addr: c.Telemetry.HTTP, Handler: telemetryServ} |
There was a problem hiding this comment.
I think defer httpServer.Close() should be called on each server in the main func as a general cleanup step and as a last resort to graceful shutdown.
cmd/dex/serve.go
Outdated
| logger.Errorf("gracefully close (%s) listener: %v", name, err) | ||
| } | ||
|
|
||
| if err := srv.Shutdown(context.Background()); err != nil { |
There was a problem hiding this comment.
What happens if there is an active connection for an indefinite amount of time? Will this wait indefinitely? If so, we should probably add a timeout here.
There was a problem hiding this comment.
Yes, good catch. I hardcoded a one-minute timeout here. Please tell me if you want to make this configurable or increase the value.
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
nabokihms
force-pushed
the
graceful-shutdown
branch
from
6687b5d to
65a8bf2
Compare
sagikazarmark
left a comment
sagikazarmark
left a comment
There was a problem hiding this comment.
A couple small things, but overall 👍 Thanks!
Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
nabokihms
force-pushed
the
graceful-shutdown
branch
from
c5f4450 to
6664b57
Compare
The official docker release for this release can be pulled from ``` ghcr.io/dexidp/dex:v2.28.0 ``` **Features:** - Add c_hash to id_token, issued on /auth endpoint, when in hybrid flow (dexidp#1773, @HEllRZA) - Allow configuration of returned auth proxy header (dexidp#1839, @seuf) - Allow to disable os.ExpandEnv for storage + connector configs by env variable DEX_EXPAND_ENV = false (dexidp#1902, @heidemn-faro) - Added the possibility to activate lowercase for UPN-Strings (dexidp#1888, @VF-mbrauer) - Add "Cache-control: no-store" and "Pragma: no-cache" headers to token responses (dexidp#1948, @nabokihms) - Add gomplate to the docker image (dexidp#1893, @nabokihms) - Graceful shutdown (dexidp#1963, @nabokihms) - Allow public clients created with API to have no client_secret (dexidp#1871, @spohner) **Bugfixes:** - Fix the etcd PKCE AuthCode deserialization (dexidp#1908, @bnu0) - Fix garbage collection logging of device codes and device request (dexidp#1918, @nabokihms) - Discovery endpoint contains updated claims and auth methods (dexidp#1951, @nabokihms) - Return invalid_grant error if auth code is invalid or expired (dexidp#1952, @nabokihms) - Return an error to auth requests with the "request" parameter (dexidp#1956, @nabokihms) **Minor changes:** - Change default themes to light/dark (dexidp#1858, @nabokihms) - Various developer experience improvements - Dependency upgrades - Tons of small fixes and changes
2 participants
Signed-off-by: m.nabokikh maksim.nabokikh@flant.com
Overview
Add graceful shutdown mechanism to all servers:
What this PR does / why we need it
Closes #1942
Does this PR introduce a user-facing change?